fix: refactor labels #1145

3em · 2025-02-21T11:42:18Z

Adjusted multiple components to the design
Added missing functionalities:

remove label at PR page
navigate to edit labels from the label dropdown
Fixed the Icon component size: in flex containers, it could shrink

Design changes (before/after):

CLAassistant · 2025-02-21T11:42:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

netlify · 2025-02-21T11:42:35Z

✅ Deploy Preview for harness-design ready!

Name	Link
🔨 Latest commit	`b9f6a26`
🔍 Latest deploy log	https://app.netlify.com/sites/harness-design/deploys/67c0ad3bfac4f90008b7c8f8
😎 Deploy Preview	https://deploy-preview-1145--harness-design.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-02-21T11:42:42Z

✅ Deploy Preview for harness-xd-review ready!

Name	Link
🔨 Latest commit	`b9f6a26`
🔍 Latest deploy log	https://app.netlify.com/sites/harness-xd-review/deploys/67c0ad3bc681e60008e124d2
😎 Deploy Preview	https://deploy-preview-1145--harness-xd-review.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

apps/gitness/src/pages-v2/pull-request/pull-request-compare.tsx

abhinavrastogi-harness · 2025-02-21T23:08:52Z

apps/gitness/src/pages-v2/repo/labels/hooks/use-full-fill-label-store.ts

i'm not sure I follow why this file/hook exists in the first place.
cc @cjlee01 @sans-harness can we discuss once?

This hook was created to populate data in the store (useLabelsStore). It's used on the labels list page and on the repository label details page. Essentially, it's just isolated code moved into a hook to fill the store, avoiding repetitive code.

apps/gitness/src/pages-v2/project/labels/hooks/use-get-project-label-and-values-data.ts

apps/gitness/src/pages-v2/project/labels/project-labels-list-container.tsx

apps/gitness/src/pages-v2/project/stores/labels-store.tsx

apps/gitness/src/pages-v2/repo/labels/hooks/use-full-fill-label-store.ts

apps/gitness/src/pages-v2/repo/labels/label-form-container.tsx

knagurski · 2025-02-24T18:36:33Z

packages/ui/src/components/icon.tsx

+      width={width || size}
+      height={height || size}
+      className={className}
+      style={{ minWidth: `${width || size}px`, minHeight: `${height || size}px` }}


Just curious why this was needed?

The Icon component is often used in layouts in a way that unintentionally reduces its width (e.g., due to flex), which is incorrect behavior. This code ensures that the icon maintains its width in any layout.

packages/ui/src/components/label-marker.tsx

knagurski · 2025-02-24T18:47:44Z

packages/ui/src/components/label-marker.tsx

+          >
+            {!!value && <span className="max-w-full truncate">{value}</span>}
+
+            {!!counter && counter}


I assume this means that counting from 0 is not allowed?

yes you are right, 0 not allowed

packages/ui/src/components/label-marker.tsx

knagurski · 2025-02-24T18:50:19Z

packages/ui/src/components/label-marker.tsx

      )}
-    </span>
+
+      {isDynamic && <Icon name="circle-plus" size={12} className="min-w-3 text-icons-4" />}


Does this have any meaning that as screen reader should be made aware of?

knagurski · 2025-02-24T18:52:06Z

packages/ui/src/components/search-box.tsx

        )}
+
        {hasShortcut && !!shortcutLetter && (
          <div className="absolute right-1.5 top-1/2 flex h-5 -translate-y-1/2 cursor-pointer items-center gap-0.5 rounded-sm border bg-background-3 px-1 text-foreground-2 duration-100 ease-in-out">
            <Icon name="apple-shortcut" size={12} />


This is going to annoy everyone who's not on a Mac.

@knagurski that's a fair point :) But I think it'd be better to move this to a separate task, if you don't mind?

Yeah, totally, just an observation when we were in the area

packages/ui/src/views/labels/components/label-cell-content.tsx

packages/ui/src/views/labels/components/label-form-color-and-name-group.tsx

packages/ui/src/views/labels/components/labels-list-view.tsx

packages/ui/src/views/labels/labels-list-page.tsx

knagurski · 2025-02-28T11:50:52Z

apps/gitness/src/pages-v2/pull-request/pull-request-conversation.tsx

+    if (!branchError) return
+
+    if (pullReqMetadata?.merged || pullReqMetadata?.closed) {
+      return setShowRestoreBranchButton(true)


Best to only return cleanup functions from a useEffect. This will be fine, but it's probably best to separate the call and the return since they aren't really connected.

Suggested change

return setShowRestoreBranchButton(true)

setShowRestoreBranchButton(true)

return

knagurski · 2025-02-28T11:57:09Z

apps/gitness/src/pages-v2/repo/labels/hooks/use-full-fill-label-store.ts

@@ -13,14 +13,16 @@ export interface UseFullFillLabelStoreProps {
 export const useFullFillLabelStore = ({ queryPage, query, enabled = true }: UseFullFillLabelStoreProps) => {


Quick question about the naming... is it "full fill" as in two words, or "fulfil" as in to complete something? Maybe it would be more obvious what the purpose of the hook was as something like usePopulateLabelsStore or usePrimeLabelsStore?

knagurski · 2025-02-28T12:04:08Z

packages/ui/src/components/label-marker.tsx

 import { cn } from '@utils/cn'
-import { ColorsEnum } from '@views/labels'
+import { ColorsEnum, ILabelType, LabelType } from '@views/labels'


Can we find a way to avoid references to the views from within components? Ideally this should be one-way to make it clear that views bring components together into a cohesive interface, leaving components as independent building blocks. Perhaps should LabelMarker be a subcomponent of the Labels view?

knagurski · 2025-02-28T12:08:03Z

packages/ui/src/components/label-marker.tsx

-        <span
-          className={`flex h-full items-center overflow-hidden rounded-r bg-label-background-black pl-1.5 pr-[7px] ${inTable && 'max-w-[50%]'}`}
+    <div className={cn('flex max-w-full items-center gap-2 opa relative', className)}>
+      <span


This being a span (an inline element), but being set as display grid (a block level element) doesn't really make much sense to me. Is there a reason we can't use a div? Makes no difference, but I'm curious if there's a reason as I've seen inline elements being changed to block a lot in this codebase 🤔

knagurski · 2025-02-28T12:10:49Z

packages/ui/src/components/label-marker.tsx

+  const isWithExtraContent = !!value || !!counter
+  const isWithDeleteButton = !!onDelete


You can totally ignore this since it makes zero difference... instead of isWith, has reads better IMO

Suggested change

const isWithExtraContent = !!value || !!counter

const isWithDeleteButton = !!onDelete

const hasExtraContent = !!value || !!counter

const hasDeleteButton = !!onDelete

knagurski · 2025-02-28T12:19:26Z

packages/ui/src/components/table.tsx

@@ -43,7 +43,7 @@ TableHeader.displayName = 'TableHeader'

 const TableBody = forwardRef<HTMLTableSectionElement, HTMLAttributes<HTMLTableSectionElement>>(
  ({ className, ...props }, ref) => (
-    <tbody ref={ref} className={cn('[&_tr:last-child]:border-0', className)} {...props} />
+    <tbody ref={ref} className={cn('[&_tr:last-child]:border-0 [&>tr:hover]:bg-background-4', className)} {...props} />


To me a change of colour on hover denotes that something is clickable. This might be the ideal for the usage in the labels work, but I don't think we can make that call for all tables. Can we make this opt-in as, perhaps, an extra prop?

knagurski · 2025-02-28T14:48:48Z

packages/ui/src/views/repo/pull-request/components/labels/pull-request-labels-header.tsx

+    return label.assigned_value?.id ?? undefined
+  }
+
+  return undefined


No need to manually return undefined; if you don't return anything, you'll get the same

Suggested change

return undefined

3em requested a review from knagurski as a code owner February 21, 2025 11:42

ankormoreankor mentioned this pull request Feb 21, 2025

refactor: labels pixel-point/canary#26

Closed

abhinavrastogi-harness reviewed Feb 21, 2025

View reviewed changes